Skip to content

Properly validate JS->native method calls#23658

Closed
aleclarson wants to merge 1 commit intofacebook:masterfrom
aleclarson:patch-2
Closed

Properly validate JS->native method calls#23658
aleclarson wants to merge 1 commit intofacebook:masterfrom
aleclarson:patch-2

Conversation

@aleclarson
Copy link
Copy Markdown
Contributor

@aleclarson aleclarson commented Feb 26, 2019

Summary

Between invalidating a bridge and suspending its JS thread, native modules may have their methods called.

Only warn when a native module has been invalidated, which happens right before its JS thread is suspended.

Avoid initializing a native module's instance if its bridge is invalidated.

/cc @fkgozali f945212#commitcomment-32467567

Changelog

[iOS] [Fixed] - Properly validate JS->native method calls

Test Plan

N/A

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 26, 2019
hramos referenced this pull request Feb 26, 2019
…id bridge

Summary: A bunch of flows including JS reload and e2e tests seem to hit the race condition, causing redbox. For now, make it a warning to unblock.

Differential Revision: D9327418

fbshipit-source-id: a72b378d88f7566268fd9415fbd34225c8b931e7
Comment thread React/CxxBridge/RCTCxxBridge.mm Outdated
Comment thread React/Base/RCTModuleData.mm Outdated
@aleclarson
Copy link
Copy Markdown
Contributor Author

aleclarson commented Feb 28, 2019

I've removed the warning entirely, in favor of silently ignoring method calls with no method queue.

The methodQueue getter returns nil when (1) the bridge is invalid and (2) the _methodQueue ivar is nil.

@matthargett
Copy link
Copy Markdown
Contributor

I would prefer to have a warning, even just in dev mode. We’ve seen things like this happen when there’s memory corruption issues within JSC or CxxReact. Being able to catch the first symptoms earlier so a dump can be created and analyzed would help.

Between invalidating a bridge and suspending its JS thread, native modules may have their methods called.

Only warn when a native module has been invalidated, which happens right before its JS thread is suspended.

Avoid initializing a native module's instance if its bridge is invalidated.
@aleclarson
Copy link
Copy Markdown
Contributor Author

@matthargett Okay, fixed.

Copy link
Copy Markdown
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fkgozali has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Mar 5, 2019
@react-native-bot
Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @aleclarson in dc89375.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added Merged This PR has been merged. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Mar 5, 2019
calebmer pushed a commit to calebmer/react-native that referenced this pull request May 23, 2019
Summary:
Between invalidating a bridge and suspending its JS thread, native modules may have their methods called.

Only warn when a native module has been invalidated, which happens right before its JS thread is suspended.

Avoid initializing a native module's instance if its bridge is invalidated.

/cc fkgozali facebook@f945212#commitcomment-32467567

[iOS] [Fixed] - Properly validate JS->native method calls
Pull Request resolved: facebook#23658

Differential Revision: D14287594

Pulled By: fkgozali

fbshipit-source-id: 89dd1906a0c55f3f48ba4ff220aac0cddf2eb822

# Conflicts:
#	React/CxxModule/RCTNativeModule.mm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants